I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

Thanks for maximizing the performance and minimizing the time taken for these algorithms with everyone's favorite types! 🚀 😻 🎁

Re: handling NaNs. This change has broken code which does pass NAN in the data. Applications can use this to indicate "missing" data. In the past, they were skipped. Now, it is a crash. We can craft a custom Compare function for std::min_element or for std::max_element but not for std::minmax_element.

This change has broken code which does pass NAN in the data

Can you make a more detailed report, with your data, expected results, actual results.
For now I'm not sure whether it is UB, or a valid case.

@jschroedl , I've managed to recreate this on randomized test.

As a workaroung you can define _USE_STD_VECTOR_ALGORITHMS to 0 in compiler options or before including any STL headers.

Note tha mixing NaN with few other numbers in input is an undefined behavior, as such input violates strict weak ordering.
All NaN input or NaN and only one other value input appears to be valid though.

For now I'm not sure whether it is UB, or a valid case.

Per [alg.min.max], it seems that processing NaN is UB for ranges::meow_element (due to indirect_strict_weak_order) but not for std::meow_element...

For now I'm not sure whether it is UB, or a valid case.

Per [alg.min.max], it seems that processing NaN is UB for ranges::meow_element (due to indirect_strict_weak_order) but not for std::meow_element...

Isn't [alg.sorting]/3 covering this on a higher level?

As a workaroung you can define _USE_STD_VECTOR_ALGORITHMS to 0 in compiler options or before including any STL headers.

Or you can define _USE_STD_VECTOR_FLOATING_ALGORITHMS to 0 to skip only this vectorization, but keep others.

For now I'm not sure whether it is UB, or a valid case.

Per [alg.min.max], it seems that processing NaN is UB for ranges::meow_element (due to indirect_strict_weak_order) but not for std::meow_element...

Isn't [alg.sorting]/3 covering this on a higher level?

Thanks for correction, and it's sure that UB is also present std::meow_element.
(Off-topic: it's may be a defect that currently [alg.sorting.general]/3 doesn't seem applied to ranges versions of algorithms in [alg.binary.search]. Thanks for notification!)

As a workaroung you can define _USE_STD_VECTOR_ALGORITHMS to 0 in compiler options or before including any STL headers.

Or you can define _USE_STD_VECTOR_FLOATING_ALGORITHMS to 0 to skip only this vectorization, but keep others.

The scenario of having NaN in collections of double is not so far-fetched as it can be good indications during telemetry or data collection as a sensor going offline, for example. I'm sure you can imagine other scenarios. We have used NaN to indicate 'missing' data for many many years and portably (mac, win) have been able to use these algorithms and have NaN skipped so our use of this is historical. Clearly, we'll need to revisit this usage.

As for the crash, a simple test case I have is the following will crash now whereas prior it would return 0, 99 for min, max.

   std::vector<double> data { 0, 1, NAN, 98, 99 };
   double dMin = *std::min_element(begin(data),end(data));
   std::cout << "\nMin is " << dMin;

   double dMax = *std::max_element(begin(data), end(data));
   std::cout << "\nMax is " << dMax;

Thank you for the workaround flag, I will investigate using that.

Thanks for providing the test case.
Looks like the circumstances of the bug is different from what I thought initially - it crashes inside the algorithm instead of returning a bogus index.

Note that your repro has UB, as @frederick-vs-ja and me have discussed just above.

The non-UB equivalent repro looks like this to me:

#include <iostream>
#include <vector>
#include <limits>

int main()
{

	std::vector<double> data{ 1, 1, NAN, 1, 1 };
	double dMin = *std::min_element(begin(data), end(data));
	std::cout << "\nMin is " << dMin;

	double dMax = *std::max_element(begin(data), end(data));
	std::cout << "\nMax is " << dMax;
}

I intend to fix this to output the following:

Min is 1
Max is 1

But I do not intend to fix your example, it may or may not be fixed.

As a proper fix on your side, I suggest having hand-written loops instead of using STL algorithms to avoid undefined behavior.
As a quick fix, defining _USE_STD_VECTOR_FLOATING_ALGORITHMS to 0 should do.

@jschroedl , the issue you reported resloved as wontfix -- see #4731

However it made me to look into +0.0 / -0.0 situation, for which I found and fixed bugs in #4734 , and it happens to fix your repro as well, though it might not fix all cases, as I didn't do thorough testing.

@AlexGuteniev, what makes things worse with crashes on NaNs is that it changes existing behavior (<= MSVC 17.9) and causes a crash that can't even be caught via a catch (...) (see https://gcc.godbolt.org/z/qzveTd8oK). We had an issue with std::minmax_element that suddenly crashed an otherwise working program after an upgrade to 17.10 and if this change persists we need to re-evaluate any *_element code that handles user data (which may well contain NaNs and is valid data in our case).
We were lucky to find this issue during testing, but it may have easily be overlooked and first appeared at a customer - maybe in a critical use case. IMHO, an optimization that breaks existing behavior and potentially crashes should only be allowed via explicit compile-time flags/defines. Maybe it should require to use #define _USE_STD_VECTOR_FLOATING_ALGORITHMS 1 explicitly.

@matthiasstraka ,

I admit I didn't expect crashes on NANs when doing this change, only wrong results.
Still I'm not sure if the optimization should be disabled or removed, and I don't have the authority to decide this.

Generally, optimizations indeed shouldn't break existing code, especially into a runtime crash.
However, if the existing code was incorrect and just happened to work, then breaking it may be acceptable.

This reminds me of a precedent when on one of platforms memcpy was defined as memmove, and then there was an optimization to take advantage of non-oevrlapped ranges, and it broke uses of memcpy with overlapped ranges. The optimization wasn't incorrect, as memcpy is specifically distinct from memmove to enable such optimizations.

The case with _element is only different from the memcpy precedent is only in that I don't know if the undefined behavior for non-strict-weak-ordered values exists to enable optimization, or for other reasons. But then again, there are precedents of using undefined behavior existing for other reasons, like signed integer overflow optimizations.

So I generally don't think that the breakage of some not well defined code necessarily justifies the revert of the optimization.

Still I think in some cases breaking of not well defined code may be fixed:

1. If the break is massive, everyone was doing the right thing, then maybe should avoid such impact, and keep the existing code working.
2. If it is possible to achieve the desired or close to the desired effect without breaking the old code, maybe should do that

For 1 I don't have figures. So far we have at least few complains. Is this massive? Probably not yet.
For 2 -- I think it may be possible.

What currently prevents me from submitting a fix which keeps the optimization, but makes NAN values don't crash it is:

• The maintainers' position on the issue <algorithm>: minmax family are returning wrong match index and may crash #4731 (comment)

  We talked about this at the weekly maintainer meeting and decided that the cases of all-nans or all-nans-except-one-plain-value aren't worth worrying about, and the Standard is vague on whether they're UB.

  I find this reasonable from some point, because it doesn't encourage not well defined code and avoid spending resources to non-goals

• The pull request for +0.0 / -0.0 fix Floating minmax: fix negative zero handling and dedicated test coverage for arrays of +0.0 and -0.0 only #4734 is in review. and before it is accepted or rejected, it is counter-productive to work on similar changes is the same area, having to duplicate some pieces, and creating conflicts to resolve. Note that currently the +0.0 / - 0.0 fix as it is currently written may fix the NAN crash too. It fixes at least some of occurrences, I just don't have exhaustive coverage to say for sure that it fixes all of them.

Since we are working with the C++17 standard at the moment, I looked up the C++17 standard-definition of max_element (§ 28.7.8):

template constexpr ForwardIterator max_element(ForwardIterator first, ForwardIterator last);
Returns: The first iterator i in the range [first, last) such that for every iterator j in the range [first, last) the following corresponding conditions hold: !(*i < *j) or comp(*i, *j) == false. Returns last if first == last.

This of course has the consequence, that if the first element of the range is NaN, a valid max cannot be found. But it is nevertheless well-defined behavior and should not lead to unexpected crashes. I'm not sure if C++20 allows to change that by requiring the indirect_strict_weak_order concept. But as I see it, C++17 and earlier does not require us to expect undefined behavior.

It is undefined behavior according to C++17 you linked as well.
See "28.7 Sorting and related operations", paragraphs 1 though 4.